Skip to content

Filter stack and radial covar optimizations#1388

Open
garrettwrong wants to merge 72 commits into
developfrom
rco_filter_stack
Open

Filter stack and radial covar optimizations#1388
garrettwrong wants to merge 72 commits into
developfrom
rco_filter_stack

Conversation

@garrettwrong

Copy link
Copy Markdown
Collaborator

Converts code towards filter stacks while maintaining ability to use legacy single filters (for now).

Provides a to_radial method for Filters.

Provides means to mix most singleton and stack filters. Vectorizes bulk evaluations.

Tries to vectorize (and sometimes use GPU when things fit) for filter_statck_to_basis_mats.

Add code paths to run radially optimized filter_stack_to_basis_mat.

Adds a smoke/timing test for covar2d using 10028 (ribsome) and 11618 (ctf per image) experimental sized cases.

Still a WIP, need self review.

@garrettwrong garrettwrong added enhancement New feature or request Optimization Performance or Resource Optimzation GPU labels Jun 11, 2026
@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.03504% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.65%. Comparing base (20a8ac2) to head (39bcdb4).

Files with missing lines Patch % Lines
src/aspire/operators/filters.py 96.02% 6 Missing ⚠️
src/aspire/basis/fle_2d.py 93.02% 3 Missing ⚠️
src/aspire/basis/ffb_2d.py 95.65% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1388      +/-   ##
===========================================
+ Coverage    90.56%   90.65%   +0.08%     
===========================================
  Files          135      135              
  Lines        14691    14926     +235     
===========================================
+ Hits         13305    13531     +226     
- Misses        1386     1395       +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@garrettwrong
garrettwrong force-pushed the rco_filter_stack branch 2 times, most recently from 1102a7e to 5b5ea36 Compare June 22, 2026 18:47
@garrettwrong

Copy link
Copy Markdown
Collaborator Author

Going to kick off the full CI suite.

@garrettwrong
garrettwrong marked this pull request as ready for review July 8, 2026 13:57
@garrettwrong
garrettwrong requested a review from janden as a code owner July 8, 2026 13:57
@garrettwrong
garrettwrong requested a review from j-c-c July 8, 2026 14:36

@janden janden left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! A few questions here and there, but nothing major.

"""
Expands radial vector or stack of vetors `radial_vec` to basis matrix.

:param radial_vec: Array holding radial vector,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how do we know whether these radial pts are compatible with the radial grid used here (from _precomp["gl_nodes"])?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the radial case, gl_nodes did not work well as-is, I had to remake them in double precision :/

# Weights appear a little sensitive to dtype, otherwise could use self._precomp["gl_nodes"]


I changed the occurrence of 0.5 to self.kcut in (one new, one existing)lgwt calls. kcut is fixed to 0.5. That would more/less enforce self consistency within the basis.


As far as the input to the function... ASPIRE code is generating the radial vectors by using basis._filter_pts. If that attribute doesn't exist the code I wrote to generate the radial vector will not succeed and the code will not reach this point. Now, If a user is cooking their own radial vector using their own code, that is their responsibility. They can read the code.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added

See `_filter_pts` for point set. 

to both expand_radial_vec docstrings.

for _ in radial_vec
]

ind_ell = 0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of the work here seems to be doubling up what's done in filter_stack_to_basis_mats. Would refactoring be an option here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. The first half of those methods are totally different, but they are similar after that. I'll try seeing what calling expand_radial_vec looks like after the averaging to a 1d vector in _filter_stack_to_basis_mats.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that seemed to work out fine. I'll try similar for FLE.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. So I was able to make both FFB and FLE call their own expand_radial_vec right after their 2d to 1d averaging. I had to modify the FLE functions a little to do this, as their expand_radial_vec was doing some additional Fourier padding by default.

Comment thread src/aspire/image/image.py Outdated
)

def filter(self, filter):
def convolve(self, filter_values):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having both filter and convolve can be confusing since they basically describe the same operation. How about calling this one _filter and calling it from filter as done below?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

Comment thread src/aspire/image/image.py Outdated
:param filter: An object of type `Filter`.
:return: A new filtered `Image` object.
"""
# Note image and filter data is intentionally migrated via

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is stale. The xp.asarray migration happens above in convolve. The last sentence is also missing parts.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, my bad, that was sloppy on my part. I must have got distracted. Fixed.

"""
return 1

def _ctf_params(self):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these still needed? I don't recall seeing them used anywhere.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally wrote it as a bridge (before the filter stack conversion). I left it for three minor reasons.

It ended up making a call in Simulation cleaner/simpler. filter_stack_params = self.filter_stack._ctf_params()[ :, :6 ] replaces a lookup loop over starfile mapping/attributes before the data is being repacked from per-filter to per-image for the set-metadata call. (The mapping still exists, its just implicit in the structure now.) Ie, it moves the lookup/packing of the filter attributes from Simulation to Filter code, which is Filter business imo. Then the only thing that happens in Simulation is the ctf to image mapping, which is Simulation business.

If anyone ever "just wants all the ctf params" It's way more convenient one-liner than dropping the starfile (gross) or duplicating code that teases them out one-by-one (ie someone copying the loop code out of from Simulation, which I've had to do several times now. For example...).

I use it to add tests for equality/inequality of {Radial}CTFFilters which is useful for debugging.

I can revert it if non of those seem worth having the code change.

Comment thread src/aspire/operators/filters.py Outdated

return CTFFilter(
self.voltage[items],
# self.wavelength[items],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure! missed that.

# Additionally we upcast so downstream computations remain in doubles.

# First prepare arrays for broadcasting.
voltage = xp.atleast_1d(voltage)[:, None]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be np since we haven't moved over to GPU yet?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PR I added an agnostic atleast_1d that will return the type of array (np/cp) it is given. If we're in xp as cupy mode, that will be the one used here.

The way it is written, ctf_formula should accept the inputs on host, gpu, or a mix and probably not crash.

Reverting would limit the param inputs to host only. I'd leave it.

@garrettwrong

Copy link
Copy Markdown
Collaborator Author

@janden for the _ctf_params and atleast_1d, just let me know what you'd prefer to see in the code and I'll change it if need be. Then I can have this back to you while I re-run all the tests. (That will take a while unfortunately).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request GPU Optimization Performance or Resource Optimzation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants